[REST API] Use case to generate and delete Application password #8400

selanthiraiyan · 2022-12-14T03:25:38Z

Part of: #8394

Description

Adds an implementation of ApplicationPasswordUseCase that works with WPCOM credentials.
Unit tests will be added in a future PR.

Testing instructions

CI passing should be enough.
The use case will be used in future PRs. No testing steps available now.

Screenshots

NA

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…sword.

…ration and deletion network actions.

…n response.

…password actions.

wpmobilebot · 2022-12-14T03:41:23Z

You can test the changes from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr8400-cd306a2 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

itsmeichigo · 2022-12-14T04:06:13Z

Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift

+    ///
+    private var applicationPasswordName: String {
+        get async {
+            await UIDevice.current.model


According to the internal discussion [p1669977432588639/1669965496.085079-slack-C046HDZL87J], we should name the application in the following format: <bundle-id>.ios-app-client.<model-name>.

Done in d905e7a

itsmeichigo · 2022-12-14T04:07:56Z

Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift

+    init(siteID: Int64,
+         networkcredentials: Credentials,
+         network: Network? = nil,
+         keychain: Keychain = Keychain(service: "com.automattic.woocommerce.applicationpassword")) {


We should use the same service name for Keychain in the whole app: WooConstants.keychainServiceName.

Done in 6b63664

itsmeichigo · 2022-12-14T04:34:04Z

Networking/Networking/Network/ApplicationPasswordNetwork.swift

+import Foundation
+import Alamofire
+
+public class ApplicationPasswordNetwork: Network {


Could you add a comment explaining why this network is necessary?

Done in b2f3d74

itsmeichigo · 2022-12-14T04:35:37Z

Networking/Networking/Network/ApplicationPasswordNetwork.swift

+        }
+    }
+
+    @available(*, deprecated, message: "Not implemented. Use the `Result` based method instead.")


Maybe use unavailable instead of deprecated since that fits this situation better.

Suggested change

@available(*, deprecated, message: "Not implemented. Use the `Result` based method instead.")

@available(*, unavailable, message: "Not implemented. Use the `Result` based method instead.")

Using unavailable will make the method unavailable, which makes ApplicationPasswordNetwork not satisfy the Network protocol requirements.

itsmeichigo · 2022-12-14T04:36:00Z

Networking/Networking/Network/ApplicationPasswordNetwork.swift

+    @available(*, deprecated, message: "Not implemented. Use the `Result` based method instead.")
+    public func responseData(for request: URLRequestConvertible, completion: @escaping (Data?, Error?) -> Void) { }
+
+    @available(*, deprecated, message: "Not implemented. Use the `Result` based method instead.")


Similar to the above comment:

Suggested change

@available(*, deprecated, message: "Not implemented. Use the `Result` based method instead.")

@available(*, unavailable, message: "Not implemented. Use the `Result` based method instead.")

#8400 (comment)

itsmeichigo · 2022-12-14T04:36:19Z

Networking/Networking/Network/ApplicationPasswordNetwork.swift

+        Empty<Swift.Result<Data, Error>, Never>().eraseToAnyPublisher()
+    }
+
+    @available(*, deprecated, message: "Not implemented")


Similar to the above comment:

Suggested change

@available(*, deprecated, message: "Not implemented")

@available(*, unavailable, message: "Not implemented")

#8400 (comment)

itsmeichigo · 2022-12-14T04:40:31Z

Networking/Networking/ApplicationPassword/ApplicationPasswordUseCase.swift

+    /// - Returns: Generated `ApplicationPassword` instance
+    ///
+    func generateNewPassword() async throws -> ApplicationPassword {
+        let password = try await createApplicationPasswordUsingWPCOMAuthToken()


Should we handle deleting and generating the password again within this use case, or do you think it's better that it's handled from the outside? I just thought it would be more straightforward for the consumer of this use case to not have to call this method twice in this case.

I agree. Started deleting the existing password when duplicate name error occurs - 38c7dec

itsmeichigo

Thanks for the quick updates! Everything looks good to me now 🚀

mokagio · 2022-12-15T11:30:15Z

@itsmeichigo @selanthiraiyan merging #8418 here should solve the CI issue

Add missing `networking_pods` dependencies to Yosemite in `Podfile`

selanthiraiyan added 9 commits December 14, 2022 08:03

Import Keychain access into Networking layer to store application pas…

762da86

…sword.

Add ApplicationPasswordNetwork to perform application password gene…

8306481

…ration and deletion network actions.

Add ApplicationPasswordMapper to map application password generatio…

ae2f7ec

…n response.

Introduce DefaultApplicationPasswordUseCase to perform application …

c330d5f

…password actions.

Add ability to store password to keychain.

325e1d0

Generate application password and handle errors.

cacc16e

Fetch wpadmin username using WPCOM token.

de36ede

Delete application password using WPCOM authentication token.

a0b8e16

Conform to ApplicationPasswordUseCase

cdc20de

selanthiraiyan mentioned this pull request Dec 14, 2022

REST API: Implementation of ApplicationPasswordUseCase protocol using WPCOM auth token #8394

Closed

2 tasks

selanthiraiyan changed the title ~~[REST API]: Use case to generate and delete Application password~~ [REST API] Use case to generate and delete Application password Dec 14, 2022

itsmeichigo reviewed Dec 14, 2022

View reviewed changes

selanthiraiyan added 4 commits December 14, 2022 12:25

Change keychain service name to match WooCommerce.

6b63664

Change application password name format to include bundle identifier.

d905e7a

Delete password and try again when duplicatName error occurs.

38c7dec

Explain why we need ApplicationPasswordNetwork.

b2f3d74

selanthiraiyan added this to the 11.7 milestone Dec 14, 2022

selanthiraiyan added type: task An internally driven task. feature: login Related to any part of the log in or sign in flow, or authentication. labels Dec 14, 2022

selanthiraiyan requested a review from jaclync December 14, 2022 08:00

selanthiraiyan marked this pull request as ready for review December 14, 2022 08:01

selanthiraiyan requested a review from itsmeichigo December 14, 2022 08:01

itsmeichigo self-assigned this Dec 14, 2022

itsmeichigo approved these changes Dec 14, 2022

View reviewed changes

selanthiraiyan enabled auto-merge December 14, 2022 09:16

selanthiraiyan and others added 3 commits December 14, 2022 15:42

Merge branch 'trunk' into feat/8394-generate-application-password

7d98684

Send password generation and username fetching in parallel.

76a8f12

Add missing networking_pods dependencies to Yosemite in Podfile

7b5dbe0

mokagio mentioned this pull request Dec 15, 2022

Add missing networking_pods dependencies to Yosemite in Podfile #8418

Merged

Merge pull request #8418 from woocommerce/mokagio/test-8394-ci-fix

cd306a2

Add missing `networking_pods` dependencies to Yosemite in `Podfile`

selanthiraiyan merged commit c5aafe4 into trunk Dec 15, 2022

selanthiraiyan deleted the feat/8394-generate-application-password branch December 15, 2022 12:13

	@available(*, deprecated, message: "Not implemented. Use the `Result` based method instead.")
	@available(*, unavailable, message: "Not implemented. Use the `Result` based method instead.")

[REST API] Use case to generate and delete Application password #8400

[REST API] Use case to generate and delete Application password #8400

Uh oh!

Conversation

selanthiraiyan commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Screenshots

Uh oh!

wpmobilebot commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsmeichigo Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsmeichigo left a comment

Choose a reason for hiding this comment

Uh oh!

mokagio commented Dec 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

selanthiraiyan commented Dec 14, 2022 •

edited

Loading

wpmobilebot commented Dec 14, 2022 •

edited

Loading

itsmeichigo Dec 14, 2022 •

edited

Loading